Repository-level data-table linting for data-manager / reference-data bundles - #23229
Draft
jmchilton wants to merge 14 commits into
Draft
Repository-level data-table linting for data-manager / reference-data bundles#23229jmchilton wants to merge 14 commits into
jmchilton wants to merge 14 commits into
Conversation
Step 1 of repository-level data-table linting (galaxyproject/planemo#1672): assemble producer/config/consumer artifacts of a data-manager repo into one model for repository-aware linters. Pure assembly, no diagnostics yet. Composes existing abstractions rather than reinventing: - manager side via convert_data_tables_xml -> DataTableBundleProcessorDescription (output_ref, table names, column maps) - configured tables + loc resolution + row-error capture via ToolDataTableManager - macro expansion via get_tool_source Tests build the model from real fixture repositories (no mocks). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Two provable hard-error checks over RepositoryDataTables: - MissingLocFixture: a configured table references a loc file that resolves to nothing (sample fallback still counts as found). - LocRowShape: a non-comment loc row can't supply every declared column index (too-few-fields / wrong-separator), reusing the row-shape errors TabularToolDataTable captures at load. Driven via lint_ctx.lint so each is skippable by name. Fixture-based tests (no mocks) cover clean / missing / broken / sample-fallback / skip cases. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Cross-component name validation over RepositoryDataTables: - ManagerTableConfigured: a data manager populates a table that no local tool_data_table config (nor known-external table) defines -> error (broken producer contract, Planemo galaxyproject#706). - ConsumerTableDefined: a literal from_data_table reference resolves to no local/known-external table -> warn (may be supplied by core or another installed repository). Only literal, fully macro-expanded names are checked; names still carrying $/@/{} markers are treated as not-checked, guarding the tools-iuc#5003 false positive where @IDX_DATA_TABLE@ looks unconfigured but resolves. external_table_names suppresses both. Fixture tests (no mocks) cover clean bundle, unconfigured manager table, externally-supplied table, unknown/known consumer, and the non-literal guard. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
OutputRefValid: a data_manager_conf output_ref that names no output of the expanded manager wrapper is a hard error, listing the real declared outputs for the fix. Only runs when the wrapper actually resolved -- ManagerDecl now carries wrapper_resolved so an unparsed wrapper leaves outputs unknown (not-checked) rather than demonstrably missing. Fixture test covers a real-output ref (clean), a bad ref (error), and the unresolved-wrapper guard. Duplicate-column-name and conflicting-schema detection deferred: a conflicting same-named table config raises inside TabularToolDataTable.merge_tool_data_table (the 26.1 assert_data_table_consistency path, outside planemo's <26.1 pin), and duplicate column names collapse in the parsed columns dict, so both need model work beyond this slice. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Unlocks the two checks deferred from step 4 via a pre-merge raw view of each <table> element (RawTableDecl: declared column names kept with duplicates, separator, comment_char, source), since the parsed columns dict collapses duplicate names and the loader *raises* when merging same-named tables with different columns. - DuplicateColumnNames (error): a <table> declares a column name twice. - ConflictingTableSchema (error): a table name is declared with differing columns/separator/comment across the bundle. build_repository_data_tables now skips loader enrichment when a column conflict is present (the loader would crash) and reports it from the raw declarations instead; table names come from the raw view too, so cross-component checks stay robust. configured_table_names unions the loader-enriched and raw declarations. Fixture tests (no mocks) cover duplicate columns, conflicting columns (assembly must not crash), conflicting separator, and a clean repo; a direct-loader check confirms the skip guard is load-bearing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Review-driven cleanups on the repository data-table bundle: - _raw_column_spec: delegate the name->index map to the canonical TabularToolDataTable.parse_column_spec_element (added by 94a027c); keep only the ordered-names-with-duplicates list DuplicateColumnNames needs. - Drop unused TableDecl.instance + RepositoryDataTables.table(); fix the stale "reused by downstream" comment (schema fields kept as honest forward surface for bundle-completeness). - Use xml_tree (house standard in the tool linters) via _tool_source_root instead of the equivalent root attribute. - Tie _has_column_conflict's docstring to the canonical loader/merge check. Add lint_repository_data_tables_bundle(lint_ctx, repo_root, ...paths...): a discovery-driven entry point that builds the model and lints it, dispatching the assembly phase through lint_ctx.lint so skip/assembly-failure diagnostics print. Lets repository linters (Planemo shed_lint) do discovery + one call. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Restore RepositoryDataTables.table(); it is covered by a step-1 model test (test_repository_data_tables.py), which the earlier removal missed. - test_list_linters: 147 -> 154 for the 7 repository data-table linters, and import bundles.lint there so the census is deterministic (matches how the test already imports every linter module). - Silence 7 Liskov [override] errors: the bundle linters intentionally target a RepositoryDataTables model, not a ToolSource. - cast the tool_data_table_confs list at the ToolDataTableManager call to work around list invariance (str paths vs list[str | PathLike]). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Relocate data-table bundle discovery (find data_manager_conf / tool_data_table_conf, walk consumer tool sources) out of Planemo into find_and_lint_repository_data_tables. Add galaxy-tool-data-lint console_script over it so a data-manager/reference-data repo can be linted standalone (--skip/--json/--fail-level). CLI tests over fixtures. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
jmchilton
force-pushed
the
data_validation
branch
from
July 31, 2026 11:51
5402051 to
f839b59
Compare
Linter.lint was annotated with ToolSource, so repository data-table linters (RepositoryDataTables target) tripped mypy [override] and needed per-method ignores. Framework already dispatches via LintTargetType TypeVar; make Linter Generic[LintTargetType] to match. Bare Linter subclasses stay Linter[Any]; bundle linters declare Linter[RepositoryDataTables]. 7 ignores removed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Real shed repos ship tool-data/<name>.loc.sample and materialize the real .loc on install. The loader's own .sample fallback strips the tool-data/ subdir and only looks in tool_data_path root, so it misses these (found=False) -- MissingLocFixture would then falsely flag nearly every reference-data repo. Add LocAsset.sample_backed (sibling <ref>.sample or tool-data/<basename>.sample) and only flag references that are neither found nor sample-backed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
list_linters() now imports the repository data-table linter module itself so the registry is populated deterministically wherever it's called (incl. planemo's --skip name validation), instead of relying on a side-effecting import in the tool-linter test. Drop that import; count stays 154 (147 tool + 7 repository linters). Add a README describing the lint fixture repos. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…efault find_and_lint seeds external_table_names with the tables Galaxy core / a stock data manager supply on essentially every deployment (all_fasta, fasta_indexes, __dbkeys__), so ConsumerTableDefined does not warn on ordinary from_data_table references to them. Callers can extend the set for repository-specific suppliers. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Warn on a .loc / .loc.sample that is empty and carries no comment describing the column format. A dataless file with a documenting comment is the accepted convention (a data manager fills the rows on install), so only empty-and-undocumented files are flagged; warning, not error. The check reads a fresh repo-tree walk (model.loc_files), not the resolved loc_assets: the empty .loc.sample files this targets never resolve into loc_assets because the loader's .sample fallback misses the shed tool-data layout. Across tools-iuc this flags 32 files (18 test-data/*.loc, 14 *.loc.sample). Give the clean fixture's 0-byte test-data/all_fasta.loc a format header so the canonical happy-path repo passes every linter. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add core_table_consumer fixture (from IUC data_manager_bwa_mem2_index_builder): defines bwa_mem2_indexes locally, consumes core all_fasta via from_data_table. find_and_lint seeds all_fasta/fasta_indexes/__dbkeys__ so the core ref doesn't warn; without the seeding it does (non-vacuous guard). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
jmchilton
force-pushed
the
data_validation
branch
from
July 31, 2026 11:58
f839b59 to
c8feba5
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements the repository-level lint block proposed in galaxyproject/planemo#1672:
validates a data-manager / reference-data repository as a connected bundle
(
data_manager_conf.xml+tool_data_table_conf.xml*+.loc/.loc.samplefixtures + consumer
from_data_tablereferences), rather than only the individualtool wrappers.
Adds eight linters, a standalone
galaxy-tool-data-lint <repo>CLI, and wiringinto Planemo
shed_lint. The linters run cleanly across the entire tools-iuc tree(780 repositories) and surfaced real bugs, each fixed in its own PR. This
consolidates several long-standing lint requests: galaxyproject/planemo#96,
galaxyproject/planemo#531, galaxyproject/planemo#706, galaxyproject/planemo#869.
Architecture
Discovery and analysis are split from linting.
build_repository_data_tables(repo_root, ...)assembles a single
RepositoryDataTablesmodel — the realized bundle — from themanager configs, table configs, loc assets, and expanded consumer tool sources; the
linters are then pure functions over that model and never touch the filesystem. This
keeps each check small and independently testable, and lets one traversal feed every
linter.
Key model facts the linters key on:
foundis whatGalaxy's own loader resolves;
is_samplemarks a production loc satisfied only viathe loader's
.samplefallback;sample_backedrecognizes the real Tool Shed layout(
tool-data/<name>.loc.sample, materialized on install) that the loader's fallbackmisses. Only the combination distinguishes a genuinely missing fixture from a
reference-data repo that is correctly shipped — without it every such repo would be
falsely flagged.
loader's merge raise, so assembly falls back to raw declarations and records the
conflict as data rather than crashing;
ConflictingTableSchemathen reports it.unresolved dynamic/
@TOKEN@references are treated as "not checked," never asdemonstrably missing (the Data manager update for metaphlan tools-iuc#5003 false-positive guard).
Each linter is dispatched through
LintContext.lint(name, ...), so every check isindividually skippable by name the same way Planemo drives the tool linters.
Severity is deliberate: a broken producer/consumer contract provable from static
evidence is an error; a condition that a legitimately external supplier could
explain is a warning (see
ConsumerTableDefined). The same model drives all threeentry points —
lint_repository_data_tables(model in hand),lint_repository_data_tables_bundle(caller did discovery), andfind_and_lint_repository_data_tables(one-call discover-and-lint, used by the CLI andshed_lint).Linters
tool_data_table_confentryoutput_refnames a real output of the expanded manager wrapperfrom_data_tabletable is defined locally or supplied externally.loc/.loc.samplethat is empty carries a format commentAll eight pass across every tools-iuc repository that declares a data-table bundle;
the row, fixture, consumer, and empty-loc linters found real defects.
LocRowShape and MissingLocFixture — proven against real repositories
LocRowShapeflags space- instead of TAB-delimited rows and truncated rows thatcannot fill the declared columns: galaxyproject/tools-iuc#8234 (checkm2 data manager),
galaxyproject/tools-iuc#8235 (dram), galaxyproject/tools-iuc#8237 (tools/checkm2),
galaxyproject/tools-iuc#8239 (coverm), galaxyproject/tools-iuc#8240 (vsnp_genbank).
MissingLocFixtureflags a configured table whose loc resolves to nothing and noshipped
.samplebacks: galaxyproject/tools-iuc#8236 (hisat2 index builder — a straybwa_mem_indexestable). Loc resolution honours the shed layout(
tool-data/<name>.loc.sample, materialized on install) so real reference-datarepositories are not falsely flagged. Addresses galaxyproject/planemo#96.
ManagerTableConfigured / OutputRefValid / DuplicateColumnNames / ConflictingTableSchema — structural
Guard rarer structural misconfigurations. All 285 bundle repositories in tools-iuc
pass them (verified by running the linters over the current tree); each is covered by
unit fixtures.
ManagerTableConfiguredaddresses galaxyproject/planemo#706 andgalaxyproject/planemo#531; the remaining three are defensive checks with no live
tools-iuc incident, exercised only by fixtures.
Representative repositories they validate cleanly:
data_manager_fetch_genome_dbkeys_all_fasta(a multi-table producer of
all_fasta+__dbkeys__),data_manager_bwa_mem2_index_builder,and
data_manager_star_index_builder(index builders whoseoutput_refcolumns andconfigured tables all resolve).
ConsumerTableDefined — undefined table references
A warning, never an error: a consumed table may legitimately be supplied by Galaxy
core or another installed repository, so an undefined reference is advisory. The
common core tables (
all_fasta,fasta_indexes,__dbkeys__) are treated asexternally supplied by default (
DEFAULT_EXTERNAL_TABLE_NAMES); callers extend theset via
external_table_names. Across tools-iuc it flagged one genuineinconsistency, fixed in galaxyproject/tools-iuc#8255:
tools/articconsumedfrom_data_table="clair3_models"but declared that table locally as
model, shippedtool-data/models.loc.samplewhile the config pointed at
model.loc, and wiredmodeltotest-data/clair3_models.loc, so its cached-model selector had no local data.EmptyLocFile — undocumented empty fixtures (galaxyproject/planemo#869)
A warning: a
.loc/.loc.samplethat is empty and carries no#comment givesno hint of its expected columns — the remedy is a one-line header, not data, so a
documented-but-dataless fixture passes. Unlike the configured-table linters this
reads a fresh repository-tree walk (
model.loc_files) rather than the loader'sresolved
loc_assets: an empty.loc.samplenever resolves into an asset (theloader's
.samplefallback misses the shedtool-data/layout), so a walk is theonly way to reach them. Across tools-iuc this flagged 32 header-less files
(14
.loc.sample, 18test-data/*.loc) out of 660; the corpus cleanup landed ingalaxyproject/tools-iuc#8256, and the 384 comment-only files already passed.
Design decisions
ConsumerTableDefinedonly walksfrom_data_tablereferences in repositoriesthat ship a bundle (a data-manager or tool-data-table config); pure-consumer tool
repositories are deliberately left uninspected. A pure consumer's table is by
definition supplied elsewhere, so flagging it as "not defined locally" would warn
on nearly every tool repo in the shed — noise, not a defect — and loading every
wrapper only earns its cost when there is a local bundle to validate against. This
is a sensible boundary for the initial pass; it can be revisited once real usage
shows whether cross-repo consumer validation (against a supplier index) is wanted.
How to test the changes?
(Select all options that apply)
Unit coverage:
test/unit/tool_util/data/test_repository_data_table_lint.py,test_repository_data_tables.py, andtest_data_lint_cli.py, driven by real fixturerepositories under
test/unit/tool_util/data/repositories/. The linters were also runover the full current tools-iuc tree (285 bundle repositories) with no crashes.
License
🤖 Generated with Claude Code